apps/jira_utils/jira_information.py (1)

153-160: Config-file precedence: consider falling back to CLI/env if keys are missing in the file

Current logic overwrites url/token with empty strings when a config file path is provided but the file lacks those keys, causing an immediate exit. If the intent is strict reproducibility, this is fine. If not, prefer config values when present, otherwise fall back to CLI/env to avoid surprising failures.

Proposed tweak:

-    if config_file_path:
-        url = config_dict.get("url", "")
-        token = config_dict.get("token", "")
+    if config_file_path:
+        # Prefer config values when provided, otherwise fall back to CLI/env
+        url = config_dict.get("url") or url
+        token = config_dict.get("token") or token

Please confirm which behavior you want long-term (strict vs. fallback).

tests/unused_code/test_unused_code.py (2)

47-76: Good coverage for fixture-used-as-parameter; consider adding a test for “commented usage”

This test validates the parameter-usage path for fixtures. To harden behavior, add a test ensuring a function name appearing only in a commented line isn’t treated as “used.” This will catch regressions around comment filtering.

I can draft a test that writes a file with a commented-out call and asserts the function is still reported unused.

---

117-126: Improve fidelity of the subprocess mock

_git_grep uses text=True, so stdout/stderr are str in reality. Returning bytes works here but diverges from actual types. Prefer strings or use subprocess.CompletedProcess for realism.

 class FakeCompleted:
     def __init__(self):
         self.returncode = 2
-        self.stdout = b""
-        self.stderr = b"fatal: not a git repository"
+        self.stdout = ""
+        self.stderr = "fatal: not a git repository"

apps/unused_code/unused_code.py (3)

34-60: Pytest fixture detection: robust and readable

Decorator handling covers both @pytest.fixture and from pytest import fixture forms, with and without parentheses. Looks good. Optional: support aliased imports (e.g., from pytest import fixture as fx) if needed in your codebase.

---

62-88: _git_grep: correct handling for rc=0/1 and unexpected errors

PCRE and untracked coverage are nice touches. Optional: add -I to ignore binary files for safety in mixed repos.

---

202-224: Exit code on processing failure: confirm desired semantics

Exceptions during file processing exit with code 2. A prior team preference (retrieved learning) was to exit(1) immediately on file read/parse failures. Do we want to standardize on 1 for any failure, or keep 1 = “unused found” vs 2 = “processing error”?

If you prefer exit(1) for these failures, I can provide a patch; otherwise, please document the exit code semantics in the CLI help for clarity.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: myakove
PR: RedHatQE/python-utility-scripts#135
File: apps/unused_code/unused_code.py:61-62
Timestamp: 2024-12-29T12:05:41.200Z
Learning: When file reading or parsing fails in apps/unused_code/unused_code.py, we want to fail (use sys.exit(1)) immediately instead of continuing.

Learnt from: myakove
PR: RedHatQE/python-utility-scripts#135
File: apps/unused_code/unused_code.py:0-0
Timestamp: 2024-12-29T11:59:22.364Z
Learning: When a function name retrieved from the AST is not found by 'git grep', the user wants a RuntimeError raised, instead of silently returning or skipping.

apps/unused_code/unused_code.py (2)

142-143: split(":", 2) still breaks on Windows paths

The earlier review flagged that git grep output should be parsed with rsplit(":", 2) to avoid mangling drive-letter paths like C:/repo/file.py:12:.... Using split from the left produces wrong _path and _line segments, so comment filtering and “own definition” checks fail again.

-    _path, _lineno, _line = entry.split(":", 2)
+    _path, _lineno, _line = entry.rsplit(":", 2)

---

160-163: Same parsing issue in fixture-parameter search

Repeat the rsplit(":", 2) fix here to keep the logic consistent and correct.

apps/unused_code/unused_code.py (1)

207-229: Thread-pool failure handling exits entire run

sys.exit(2) inside the for future in as_completed loop aborts on the first file that raises, hiding subsequent errors. Collect all failures and print a summary instead so users can fix multiple issues in one run.

tests/unused_code/test_unused_code.py (1)

65-71: Use path.as_posix() to avoid OS-specific path separators in mock

Building grep output with f"{py_file}:1:..." relies on the OS path format. For cross-platform safety (especially Windows), convert the Path to POSIX first: py_file.as_posix().

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: myakove
PR: RedHatQE/python-utility-scripts#135
File: apps/unused_code/unused_code.py:0-0
Timestamp: 2024-12-29T11:59:22.364Z
Learning: When a function name retrieved from the AST is not found by 'git grep', the user wants a RuntimeError raised, instead of silently returning or skipping.

apps/unused_code/unused_code.py (1)

203-208: Fix parsing of git-grep output; current split breaks on Windows drive letters and paths with colons

Use rsplit to reliably extract the last two fields (lineno and content). This also fixes comment filtering on the content.

-            _path, _lineno, _line = entry.split(":", 2)
+            _path, _lineno, _line = entry.rsplit(":", 2)
@@
-                _path, _lineno, _line = entry.split(":", 2)
+                _path, _lineno, _line = entry.rsplit(":", 2)

If you want, I can add a unit test asserting commented-out usages are ignored for both call-sites and fixture-parameter searches.

Also applies to: 221-228

apps/unused_code/unused_code.py (1)

272-299: Small robustness improvements for concurrency and UX

• Pre-flight grep-flag detection before spawning threads to fail fast with a clear error if unsupported.
• Optionally stabilize output ordering for deterministic CI logs.

     if not os.path.exists(".git"):
         LOGGER.error("Must be run from a git repository")
         sys.exit(1)
 
+    # Pre-flight: detect supported grep engine once and fail-fast if unsupported
+    try:
+        _detect_supported_grep_flag()
+    except RuntimeError as e:
+        LOGGER.error(str(e))
+        sys.exit(2)
+
     with ThreadPoolExecutor() as executor:

Optional deterministic output (outside this hunk):

• Before printing, split lines and sort:

if unused_functions:
    lines = "\n".join(unused_functions).splitlines()
    click.echo("\n".join(sorted(lines)))
    sys.exit(1)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: myakove
PR: RedHatQE/python-utility-scripts#135
File: apps/unused_code/unused_code.py:0-0
Timestamp: 2024-12-29T11:59:22.364Z
Learning: When a function name retrieved from the AST is not found by 'git grep', the user wants a RuntimeError raised, instead of silently returning or skipping.

apps/unused_code/unused_code.py (2)

264-273: Bug: incorrect parsing of git grep output; use rsplit to handle Windows paths with colons

split(":", 2) from the left breaks when paths contain colons (e.g., “C:...”); _line ends up prefixed with the line number, breaking comment filtering and other checks.

-            parts = entry.split(":", 2)
+            # git grep -n outputs: path:lineno:content; split from the right to handle colons in paths
+            parts = entry.rsplit(":", 2)
             if len(parts) != 3:
                 continue
-            _path, _lineno, _line = parts
+            _path, _lineno, _line = parts

Add a regression test that ensures commented-out usages (“# foo()”) are ignored even when line numbers are present.

---

295-306: Same parsing bug in fixture-parameter loop; use rsplit here as well

Mirrors the call-site parsing issue. Fix as above.

-                parts = entry.split(":", 2)
+                parts = entry.rsplit(":", 2)
                 if len(parts) != 3:
                     continue
                 _path, _lineno, _line = parts

tests/unused_code/test_doc_header_false_positives.py (4)

27-60: Test name/docstring vs assertion are contradictory

Docstrings say “should not match,” yet assertions intentionally expect a match to demonstrate the current false positive. Clarify intent in the test name/docstring to avoid confusion for future readers.

-    def test_documentation_patterns_should_not_match_pcre(self):
-        """Test that PCRE patterns don't match documentation patterns."""
+    def test_pcre_call_pattern_currently_matches_doc_lines(self):
+        """Demonstrate that the current PCRE call pattern matches documentation lines (known false positive)."""

---

181-220: Integration test is solid; add a case for commented-out usages

Good coverage of doc-only references. Add a case where the only “usage” is commented out (e.g., “# create_namespace()”) to assert it’s ignored, preventing regressions in comment filtering.

I can add a param to documentation_matches to include a commented usage and assert it’s ignored.

---

231-269: Assertion condition is weak; tighten to ensure the function isn’t reported unused

The current OR condition passes if either substring is absent anywhere in the output. Make the intent explicit.

-        assert "get_pod_status" not in result or "Is not used anywhere in the code" not in result
+        assert "get_pod_status:Is not used anywhere in the code" not in result.replace("\n", "")
# Alternatively:
+# assert "get_pod_status" not in result  # if you want a stricter check

---

347-349: Avoid test runner invocation inside test module

Running pytest from within the test file can interfere with external runners and parallelization. Recommend removing this block.

-if __name__ == "__main__":
-    # Run tests to demonstrate the issue
-    pytest.main([__file__, "-v"])

apps/unused_code/unused_code.py (3)

126-185: Documentation filter helps significantly; consider a few minor enhancements

Current heuristics are sensible. Optional improvements:

• Add “except ” and “try:” to code_prefixes to avoid misclassifying exceptional control flow.
• Doc-starters list treats any line starting with “” or “-” as doc only if it contains “name(”; acceptable, but you may further require a space after the bullet to reduce rare code edge cases (e.g., “x-foo(…)”).

-    code_prefixes = ["if ", "elif ", "while ", "for ", "with ", "assert ", "return ", "yield ", "raise "]
+    code_prefixes = ["if ", "elif ", "while ", "for ", "with ", "assert ", "return ", "yield ", "raise ", "try:", "except "]

---

275-286: Heuristics to skip def/comment/docs are correct; consider short-circuiting to used=True

After passing all filters, you can set used = True without re-checking name containment since grep matched the call pattern already.

-            if func.name in _line:
-                used = True
-                break
+            used = True
+            break

---

382-386: Aggregating per-file outputs is fine; consider splitting lines before sorting for global ordering

If you ever want fully deterministic ordering across all findings (not just per-file blocks), split lines and sort globally.

-        sorted_output = sorted(unused_functions)
-        click.echo("\n".join(sorted_output))
+        all_lines: list[str] = []
+        for block in unused_functions:
+            all_lines.extend(block.splitlines())
+        click.echo("\n".join(sorted(all_lines)))

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

apps/unused_code/unused_code.py (2)

269-272: Critical: Incorrect git grep output parsing causes comment detection to fail.

The current parsing splits the output with split(":", 2), but this extracts path, line number, and content. The comment detection on Line 279 then checks if _line.strip().startswith("#"), but _line contains the actual line content after the second colon, so this check works correctly.

However, there's a subtle issue: the code should consistently handle the parsing. The variable naming is misleading - you have _, _, _line = parts which suggests you're discarding the first two parts, but actually you need all three parts for proper validation.

Apply this fix for clarity:

-            parts = entry.split(":", 2)
-            if len(parts) != 3:
-                continue
-            _, _, _line = parts
+            parts = entry.split(":", 2)
+            if len(parts) != 3:
+                continue
+            _path, _lineno, _line = parts

---

298-301: Same parsing clarity issue in fixture parameter search.

The fixture parameter search has the same misleading variable naming, though the parsing itself is correct.

The code already correctly names the variables on Line 301, so this is actually fine. The variables are properly named as _path, _lineno, _line = parts.

apps/unused_code/unused_code.py (2)

157-158: Documentation pattern detection may have false negatives.

The regex pattern rf"^\s+{re.escape(function_name)}\s*\([^)]*\)\s*:\s+\w" requires indentation at the start (^\s+). This might miss documentation patterns that start at the beginning of a line without indentation.

Consider making the indentation optional:

-    if re.search(rf"^\s+{re.escape(function_name)}\s*\([^)]*\)\s*:\s+\w", stripped_line):
+    if re.search(rf"^\s*{re.escape(function_name)}\s*\([^)]*\)\s*:\s+\w", stripped_line):

---

359-365: Remove commented-out debug code before merging.

There's commented-out debug code that appears to be for testing purposes. This should be removed to keep the codebase clean.

-    # res = process_file(
-    #     py_file="tests/unused_code/unused_code_file_for_test.py",
-    #     func_ignore_prefix=func_ignore_prefix,
-    #     file_ignore_list=file_ignore_list,
-    # )
-    # __import__("ipdb").set_trace()
-    # return

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

apps/unused_code/unused_code.py (1)

465-469: Safer split of git grep output to handle rare path colons

Use rsplit(":", 2) to split from the right; this avoids edge cases if a path ever contains a colon (less likely on Windows due to relative paths, but still safer).

-            parts = entry.split(":", 2)
+            parts = entry.rsplit(":", 2)
@@
-                    parts = entry.split(":", 2)
+                    parts = entry.rsplit(":", 2)

Also applies to: 498-505

apps/utils.py (2)

76-89: Prune excluded dirs in-place and normalize directory path for consistent, faster walks

• Use in-place pruning of dirs to avoid descending into excluded trees; current substring check still traverses them and can be slow on large repos.
• Normalize provided directory to an absolute path for consistent outputs regardless of CLI input.
• Consider adding ".venv" to the excludes (fairly common).

Apply:

 def all_python_files(directory: click.Path | None = None) -> Iterable[str]:
     """
     Get all python files from current directory and subdirectories
     """
-    exclude_dirs = [".tox", "venv", ".pytest_cache", "site-packages", ".git"]
-    target = str(directory) if directory else os.path.abspath(os.curdir)
+    exclude_dirs = {".tox", "venv", ".venv", ".pytest_cache", "site-packages", ".git"}
+    target = os.path.abspath(str(directory)) if directory else os.path.abspath(os.curdir)
 
-    for root, _, files in os.walk(target):
-        if [_dir for _dir in exclude_dirs if _dir in root]:
-            continue
+    for root, dirs, files in os.walk(target, topdown=True):
+        # Prevent walking into excluded directories
+        dirs[:] = [d for d in dirs if d not in exclude_dirs]
         for filename in files:
             if filename.endswith(".py"):
                 yield os.path.join(root, filename)

---

76-83: Docstring should mention the optional directory argument and absolute yielding behavior

The docstring still reflects “current directory” only. Please clarify that passing directory scopes the walk root. If you accept the abspath change above, note that yielded paths are absolute.

tests/unused_code/manifests/functions_as_args.py (1)

5-9: Use typing.Callable instead of builtin callable in annotations

The builtin callable is a function, not a PEP 484 type. Use typing.Callable (optionally with ellipsis/return type). Future annotations won’t save you from static typing here.

-from __future__ import annotations
+from __future__ import annotations
+from typing import Any, Callable
@@
-class TimeoutSampler:
-    def __init__(self, wait_timeout: int, sleep: int, func: callable, **func_kwargs) -> None:
+class TimeoutSampler:
+    def __init__(self, wait_timeout: int, sleep: int, func: Callable[..., Any], **func_kwargs) -> None:

tests/unused_code/test_unused_code_coverage.py (2)

138-149: Enable the documentation-pattern test once the parser bug is fixed

Once _is_documentation_pattern correctly matches indented “name (type): ...” lines (see my comment in apps/unused_code/unused_code.py), this xfail can be removed to assert the behavior.

-@pytest.mark.xfail
 @pytest.mark.parametrize(
     ("line", "is_doc"),
     [
         ("    my_function (str): description", True),
         ("if my_function(): pass", False),
         ("def my_function(): pass", False),
         ("    # my_function()", True),
     ],
 )
 def test_is_documentation_pattern(line, is_doc):
     assert _is_documentation_pattern(line, "my_function") == is_doc

---

152-159: Assert the precise exception type from _git_grep for clearer intent

_git_grep wraps git errors in RuntimeError. Asserting Exception is broad and could mask regressions.

 with pytest.raises(Exception):
-    _git_grep("pattern")
+    _git_grep("pattern")

Follow-up:

-    with pytest.raises(Exception):
+    with pytest.raises(RuntimeError):
         _git_grep("pattern")

apps/unused_code/unused_code.py (2)

462-486: Reduce false positives: ignore string-literal-only occurrences in general usage scan

Whole-word matching is intended to catch references, but it will also treat occurrences inside string literals as “usage” (e.g., logging). For non-fixture scanning, skip lines where the match is quoted.

         # Ignore commented lines (full line or inline)
         code_part = _line.split("#", 1)[0]
         if func.name not in code_part:
             continue
+        # Ignore occurrences inside string literals like "my_function" or 'my_function'
+        if re.search(rf'(?<!\\)(["\'])\s*{re.escape(func.name)}\s*\1', code_part):
+            continue
 
         # If we are here, it's a valid usage.
         used = True
         break

Note: This does not affect fixture-specific string checks below.

---

573-580: Mutual exclusivity: make --file-path and --directory conflict explicit

Currently both options can be provided; code silently prefers file_path. Be explicit and fail fast to avoid user confusion.

     if file_path and not os.path.isfile(str(file_path)):
         LOGGER.error("File path must be a file, not a directory.")
         sys.exit(1)
 
     if directory and not os.path.isdir(str(directory)):
         LOGGER.error("Directory must be a directory, not a file.")
         sys.exit(1)
+
+    if file_path and directory:
+        LOGGER.error("Please pass only one of --file-path or --directory, not both.")
+        sys.exit(1)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

tests/unused_code/manifests/unused_code_file_for_test.py (1)

1-219: Good coverage of documentation-like patterns and real usages

This manifest thoughtfully exercises many doc patterns and legitimate calls. It should help stabilize false-positive filtering.

apps/unused_code/unused_code.py (1)

101-115: Confirm intent: usage pattern now matches any whole-word reference (not only call sites)

The new _build_usage_pattern switches from call-site to whole-word matching to pick up references (e.g., passing functions as args). This increases recall but may increase false positives (strings, comments). With the string-literal filter above and existing doc filtering, this seems reasonable. Please confirm this trade-off is intended.